[stress] stress test rerun failed jobs feature by ckairen · Pull Request #5726 · Azure/azure-sdk-tools

ckairen · 2023-03-16T22:04:15Z

azure-sdk · 2023-03-16T22:13:21Z

The following pipelines have been queued for testing:
java - template
java - template - tests
js - template
net - template
net - template - tests
python - template
python - template - tests
You can sign off on the approval gate to test the release stage of each pipeline.
See eng/common workflow

benbp · 2023-03-16T22:21:53Z

eng/common/scripts/stress-testing/deploy-stress-tests.ps1

    # Renders chart templates locally without deployment
    [Parameter(Mandatory=$False)][switch]$Template,

+    [Parameter(Mandatory=$False)][switch]$RerunFailedJobs,


I know all the wording in the original issue says "Rerun" but I'm thinking "Retry" is a better choice here?

Also it would be retrying failed pods, not jobs. Should we call it something like RetryFailedTests?

benbp · 2023-03-16T22:23:12Z

eng/common/scripts/stress-testing/stress-test-deployment-lib.ps1

+        $pods = kubectl get pods -n $pkg.namespace -o json | ConvertFrom-Json
+
+        # Get all jobs within this helm release
+        $helmResources = helm status -n $pkg.Namespace $pkg.ReleaseName --show-resources


A comment here with an example of the output would be nice.

benbp · 2023-03-16T22:23:45Z

eng/common/scripts/stress-testing/stress-test-deployment-lib.ps1

+        foreach ($helmResource in $helmResources) {
+            if ($discoveredJob -and $helmResource -match "==>") {break}
+            if ($discoveredJob) {
+                $jobs += ($helmResource -split '\s+')[0] | where{($_ -ne "NAME") -and ($_)}


Suggested change

$jobs += ($helmResource -split '\s+')[0] | where{($_ -ne "NAME") -and ($_)}

$jobs += ($helmResource -split '\s+')[0] | Where-Object {($_ -ne "NAME") -and ($_)}

what's the advantage of doing so?

"where" seems to only work on arrays & will return empty array instead of null when no matches were found

We favor using full cmdlet names to reduce confusion (see explanation). For example, where as it is used here is an alias for Where-Object and is specifically different from .Where() which I think you are describing?

benbp · 2023-03-16T22:25:14Z

tools/stress-cluster/chaos/examples/network-stress-example/Dockerfile

 # Included packages: https://github.com/microsoft/CBL-Mariner/blob/1.0/SPECS/core-packages/core-packages.spec

 ADD ./poll.sh /poll.sh
+RUN tdnf -y install wget


Ooh yay thank you

benbp · 2023-03-16T22:26:35Z

eng/common/scripts/stress-testing/stress-test-deployment-lib.ps1

+        }
+
+        if (!$genValRerun.scenarios.length) {
+            Write-Host "There are no failed jobs to rerun."


Same comment here re: naming

benbp · 2023-03-16T22:28:52Z

eng/common/scripts/stress-testing/stress-test-deployment-lib.ps1

    $genVal = Get-Content $genValFile -Raw | ConvertFrom-Yaml -Ordered
+    $releaseName = $pkg.ReleaseName
+    if ($RerunFailedJobs) {
+        $pods = kubectl get pods -n $pkg.namespace -o json | ConvertFrom-Json


Can we move this into its own helper function? That will make the outer function smaller and it will be easier to tell which variables are set/updated from this scope that are important for the remaining function code.

azure-sdk · 2023-03-23T18:44:33Z

The following pipelines have been queued for testing:
java - template
java - template - tests
js - template
net - template
net - template - tests
python - template
python - template - tests
You can sign off on the approval gate to test the release stage of each pipeline.
See eng/common workflow

benbp · 2023-03-23T21:19:09Z

eng/common/scripts/stress-testing/stress-test-deployment-lib.ps1

    $genVal = Get-Content $genValFile -Raw | ConvertFrom-Yaml -Ordered
+    $releaseName = $pkg.ReleaseName
+    if ($RetryFailedTests) {
+        runRetryTests $pkg ([ref]$releaseName) ([ref]$genVal)


Let's just return new values instead of using refs here, to reduce future bug possibilities and make the code clearer.

Suggested change

runRetryTests $pkg ([ref]$releaseName) ([ref]$genVal)

$releaseName, $genVal = runRetryTests $pkg $releaseName $genVal

Also the naming here is still confusing. runRetryTests doesn't actually run anything, it just generates the new $genVal value. $genVal is also still a non-obvious name for this variable, maybe something more along these lines:

$genValFile -> $generatedHelmValuesFilePath
$genVal -> $generatedHelmValues

benbp · 2023-03-23T21:21:07Z

eng/common/scripts/stress-testing/stress-test-deployment-lib.ps1

+    $pods = kubectl get pods -n $pkg.namespace -o json | ConvertFrom-Json
+
+    # Get all jobs within this helm release
+    $helmResources = helm status -n $pkg.Namespace $pkg.ReleaseName --show-resources


Suggested change

$helmResources = helm status -n $pkg.Namespace $pkg.ReleaseName --show-resources

$helmResources = RunOrExitOnFailure helm status -n $pkg.Namespace $pkg.ReleaseName --show-resources

benbp · 2023-03-23T21:22:30Z

eng/common/scripts/stress-testing/stress-test-deployment-lib.ps1

+    foreach ($helmResource in $helmResources) {
+        if ($discoveredJob -and $helmResource -match "==>") {break}
+        if ($discoveredJob) {
+            $jobs += ($helmResource -split '\s+')[0] | Where-Object {($_ -ne "NAME") -and ($_)}
+        }
+        if ($helmResource -match "==> v1/Job") {
+            $discoveredJob = $True
+        }
+    }


Another suggestion for clearer variable names:

Suggested change

foreach ($helmResource in $helmResources) {

if ($discoveredJob -and $helmResource -match "==>") {break}

if ($discoveredJob) {

$jobs += ($helmResource -split '\s+')[0] | Where-Object {($_ -ne "NAME") -and ($_)}

}

if ($helmResource -match "==> v1/Job") {

$discoveredJob = $True

}

}

foreach ($line in $helmStatusOutput) {

if ($discoveredJob -and $line -match "==>") {break}

if ($discoveredJob) {

$jobs += ($line -split '\s+')[0] | Where-Object {($_ -ne "NAME") -and ($_)}

}

if ($line -match "==> v1/Job") {

$discoveredJob = $True

}

}

benbp · 2023-03-23T21:23:31Z

eng/common/scripts/stress-testing/stress-test-deployment-lib.ps1

+            $revision = $jobRevision
+        }
+
+        $podPhase = kubectl describe jobs -n $pkg.Namespace $job | Select-String "0 Failed"


Suggested change

$podPhase = kubectl describe jobs -n $pkg.Namespace $job | Select-String "0 Failed"

$jobOutput = RunOrExitOnFailure kubectl describe jobs -n $pkg.Namespace $job

$podPhase = $jobOutput | Select-String "0 Failed"

benbp · 2023-03-23T21:24:01Z

eng/common/scripts/stress-testing/stress-test-deployment-lib.ps1

 }
+
+function runRetryTests ($pkg, [ref]$releaseName, [ref]$genVal) {
+    $pods = kubectl get pods -n $pkg.namespace -o json | ConvertFrom-Json


Suggested change

$pods = kubectl get pods -n $pkg.namespace -o json | ConvertFrom-Json

$podOutput = RunOrExitOnFailure kubectl get pods -n $pkg.namespace -o json

$pods = $podOutput | ConvertFrom-Json

benbp · 2023-03-23T21:26:11Z

Just some exit handling and naming comments otherwise this is looking good. I think because the variables are untyped and getting used across so much code we need to be extra careful that the variable names describe their underlying values accurately.

benbp · 2023-03-27T18:14:41Z

eng/common/scripts/stress-testing/test.ps1

@@ -0,0 +1,16 @@
+
+function determineRetryTests ([ref] $val, [ref]$val2) {


Did you mean to check this in? If so let's make it a pester test: https://github.com/Azure/azure-sdk-tools/blob/main/doc/development/powershell.md#unit-testing

oh no, accident, it's my testing ps script

azure-sdk · 2023-03-27T18:15:24Z

The following pipelines have been queued for testing:
java - template
java - template - tests
js - template
net - template
net - template - tests
python - template
python - template - tests
You can sign off on the approval gate to test the release stage of each pipeline.
See eng/common workflow

azure-sdk · 2023-03-27T18:26:48Z

The following pipelines have been queued for testing:
java - template
java - template - tests
js - template
net - template
net - template - tests
python - template
python - template - tests
You can sign off on the approval gate to test the release stage of each pipeline.
See eng/common workflow

Sync eng/common directory with azure-sdk-tools for PR Azure/azure-sdk-tools#5726 See [eng/common workflow](https://github.com/Azure/azure-sdk-tools/blob/main/eng/common/README.md#workflow) --------- Co-authored-by: Albert Cheng <albertcheng@microsoft.com>

ghost · 2023-03-28T16:40:50Z

Hello @azure-sdk!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

ckairen added 2 commits March 7, 2023 14:36

rerun flag

9e4b06f

rerun failed stress test

670a312

ckairen added Central-EngSys This issue is owned by the Engineering System team. Stress This issue is related to stress testing, part of our reliability pillar. labels Mar 16, 2023

ckairen self-assigned this Mar 16, 2023

update stress test samples wget in docker images

97d24af

ckairen marked this pull request as ready for review March 16, 2023 22:08

ckairen requested review from benbp, konrad-jamrozik and weshaggard as code owners March 16, 2023 22:08

benbp reviewed Mar 16, 2023

View reviewed changes

ckairen added 2 commits March 16, 2023 16:09

naming & commenting

d3fb0d0

update

b4d7f06

benbp reviewed Mar 23, 2023

View reviewed changes

function and var renaming for better readability

c5cf777

benbp reviewed Mar 27, 2023

View reviewed changes

readability & exit on error

6c85db2

benbp approved these changes Mar 27, 2023

View reviewed changes

azure-sdk added the auto-merge label Mar 28, 2023

ghost merged commit f6e7ccd into main Mar 28, 2023

ghost deleted the albertcheng/stress-rerun branch March 28, 2023 16:41

This pull request was closed.

	$jobs += ($helmResource -split '\s+')[0] \| where{($_ -ne "NAME") -and ($_)}
	$jobs += ($helmResource -split '\s+')[0] \| Where-Object {($_ -ne "NAME") -and ($_)}

	runRetryTests $pkg ([ref]$releaseName) ([ref]$genVal)
	$releaseName, $genVal = runRetryTests $pkg $releaseName $genVal

	$helmResources = helm status -n $pkg.Namespace $pkg.ReleaseName --show-resources
	$helmResources = RunOrExitOnFailure helm status -n $pkg.Namespace $pkg.ReleaseName --show-resources

	$podPhase = kubectl describe jobs -n $pkg.Namespace $job \| Select-String "0 Failed"
	$jobOutput = RunOrExitOnFailure kubectl describe jobs -n $pkg.Namespace $job
	$podPhase = $jobOutput \| Select-String "0 Failed"

	$pods = kubectl get pods -n $pkg.namespace -o json \| ConvertFrom-Json
	$podOutput = RunOrExitOnFailure kubectl get pods -n $pkg.namespace -o json
	$pods = $podOutput \| ConvertFrom-Json

		@@ -0,0 +1,16 @@

		function determineRetryTests ([ref] $val, [ref]$val2) {

Conversation

ckairen commented Mar 16, 2023

Uh oh!

azure-sdk commented Mar 16, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azure-sdk commented Mar 23, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benbp commented Mar 23, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azure-sdk commented Mar 27, 2023

Uh oh!

azure-sdk commented Mar 27, 2023

Uh oh!

ghost commented Mar 28, 2023

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.